Skip to content

Conversation

jonathanberthias
Copy link
Contributor

Hello! I've added a test to ensure all basic binary operations work across all 3 types of expressions: Expr, GenExpr and MatrixExpr. Here were the failures before the changes in this PR:

FAILED tests/test_matrix_variable.py::test_binop[add-genexpr-matvar] - AttributeError: 'MatrixExpr' object has no attribute 'getOp'
FAILED tests/test_matrix_variable.py::test_binop[sub-genexpr-matvar] - AttributeError: 'MatrixExpr' object has no attribute 'getOp'
FAILED tests/test_matrix_variable.py::test_binop[mul-var-matvar] - NotImplementedError
FAILED tests/test_matrix_variable.py::test_binop[mul-genexpr-matvar] - AttributeError: 'MatrixExpr' object has no attribute 'getOp'
FAILED tests/test_matrix_variable.py::test_binop[truediv-var-matvar] - AttributeError: 'MatrixExpr' object has no attribute 'getOp'
FAILED tests/test_matrix_variable.py::test_binop[truediv-genexpr-matvar] - AttributeError: 'MatrixExpr' object has no attribute 'getOp'

These failed in various places. The most common cause of errors was that GenExpr dunder methods can't handle MatrixExpr.

With the changes, all of these cases pass. Note that the test doesn't really check the result is correct, just that the operations do not crash. I expect the other tests in the test suite to ensure correctness.

Fixes #1066

jonathanberthias and others added 3 commits September 13, 2025 15:15
Currently failing:
FAILED tests/test_matrix_variable.py::test_binop[add-genexpr-matvar] - AttributeError: 'MatrixExpr' object has no attribute 'getOp'
FAILED tests/test_matrix_variable.py::test_binop[sub-genexpr-matvar] - AttributeError: 'MatrixExpr' object has no attribute 'getOp'
FAILED tests/test_matrix_variable.py::test_binop[mul-var-matvar] - NotImplementedError
FAILED tests/test_matrix_variable.py::test_binop[mul-genexpr-matvar] - AttributeError: 'MatrixExpr' object has no attribute 'getOp'
FAILED tests/test_matrix_variable.py::test_binop[truediv-var-matvar] - AttributeError: 'MatrixExpr' object has no attribute 'getOp'
FAILED tests/test_matrix_variable.py::test_binop[truediv-genexpr-matvar] - AttributeError: 'MatrixExpr' object has no attribute 'getOp'
@Joao-Dionisio
Copy link
Member

Thank you @jonathanberthias! Can you please add a CHANGELOG entry?

@Joao-Dionisio Joao-Dionisio requested a review from Copilot October 9, 2025 10:38
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes binary operations between GenExpr and MatrixExpr objects that were previously failing with AttributeError and NotImplementedError exceptions. The fix ensures that all basic arithmetic operations (add, subtract, multiply, divide) work correctly across the three expression types: Expr, GenExpr, and MatrixExpr.

  • Added explicit handling for MatrixExpr in GenExpr and Expr binary operation methods
  • Fixed division operation to check instance type before accessing getOp() method
  • Updated buildGenExprObj to properly return MatrixExpr view for matrix expressions

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tests/test_matrix_variable.py Added comprehensive test for binary operations across all expression types
src/pyscipopt/expr.pxi Fixed binary operation methods to handle MatrixExpr and added type checking for division

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

# we can't divide by 0
if divisor.getOp() == Operator.const and divisor.number == 0.0:
if isinstance(divisor, GenExpr) and divisor.getOp() == Operator.const and divisor.number == 0.0:
raise ZeroDivisionError("cannot divide by 0")
Copy link

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The division by zero check is incomplete. If divisor is not a GenExpr (e.g., a MatrixExpr), the check is bypassed and division by zero could still occur in the divisor**(-1) operation.

Suggested change
raise ZeroDivisionError("cannot divide by 0")
raise ZeroDivisionError("cannot divide by 0")
elif isinstance(divisor, MatrixExpr):
# Check if MatrixExpr is a zero matrix or scalar zero
if hasattr(divisor, "is_zero") and divisor.is_zero():
raise ZeroDivisionError("cannot divide by zero MatrixExpr")
# If MatrixExpr has a 'number' attribute and is scalar
if hasattr(divisor, "number") and divisor.number == 0.0:
raise ZeroDivisionError("cannot divide by 0")

Copilot uses AI. Check for mistakes.

Copy link

codecov bot commented Oct 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.56%. Comparing base (6452ea6) to head (aad010c).
⚠️ Report is 28 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1071      +/-   ##
==========================================
+ Coverage   54.13%   54.56%   +0.42%     
==========================================
  Files          22       23       +1     
  Lines        5045     5273     +228     
==========================================
+ Hits         2731     2877     +146     
- Misses       2314     2396      +82     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jonathanberthias
Copy link
Contributor Author

Thank you @jonathanberthias! Can you please add a CHANGELOG entry?

Done 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when adding MatrixExpr to GenExpr
2 participants